MSlenejennum/647452/new header and footer layouts and report themes - #10489
MSlenejennum/647452/new header and footer layouts and report themes#10489MSlenejennum wants to merge 37 commits into
Conversation
…upgrade Adds the reusable Composite Layout parts that ship with the Base Application - 11 header/footer designs and 3 report themes - and the code that writes them into the shared pool under Tenant Report Defaults, so an administrator can assign them to any report from the report themes and header/footer setup page. - .resources/ReportParts: the shipped layout files - Composite Report Parts Mgt. (9667): seeds the pool. Safe to call repeatedly, and each part is written in isolation so one that cannot be written is reported to telemetry and skipped rather than failing install or upgrade - BaseApp Install: seeds per database on install - Upgrade Composite Report Parts: seeds per database on upgrade, guarded by an upgrade tag that is only recorded when every part was written, so a partial seed is retried by the next upgrade - Upgrade Tag Definitions in W1, BE, IT and RU: the tag and its per-database registration Assigning the parts to report layouts is not part of this change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MSlenejennum
changed the title
github-actions
Bot
added
the
Team: Integrations
…ejennum/647452/New-Header-and-Footer-Layouts-and-Report-Themes
…ejennum/647452/New-Header-and-Footer-Layouts-and-Report-Themes
…ejennum/647452/New-Header-and-Footer-Layouts-and-Report-Themes
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
The publisher-only detail event logs raw Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4 |
|
GetCompositeReportPartsUpgradeTag() (added to UpgradeTagDefinitions in W1/BE/IT/RU) is never added to the RegisterPerDatabaseTags list (the [EventSubscriber] on Codeunit::"Upgrade Tag", 'OnGetPerDatabaseUpgradeTags') in src/Layers/W1/BaseApp/OtherCapabilities/Upgrade/UpgradeTagDefinitions.Codeunit.al, unlike every other per-database upgrade tag defined in that file (e.g. GetNewISVPlansUpgradeTag, GetSharePointConnectionUpgradeTag, etc., all of which are registered). That registration list is what the platform uses to mark tags as already-applied for brand-new installations, so upgrade logic tied to them does not redundantly run for a tenant that never needed the migration. Because 'Upgrade Composite Report Parts' is Subtype = Upgrade with OnUpgradePerDatabase, omitting the registration is inconsistent with the established pattern for every sibling tag in this same file and should be reviewed for correctness (does the platform's automatic per-database tag pre-seeding on install rely on this list to avoid a redundant SeedShippedParts() re-run through a later upgrade pass?). Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4 |
|
ClearTestReportLayouts iterates "Tenant Report Layout" without SetLoadFields before FindSet, so every field of every row is materialized even though only Name is read before the row is re-fetched for deletion. Suggested fix (apply manually — could not be anchored as a one-click suggestion): TenantReportLayout.SetRange("Report ID", BodyReportID);
TenantReportLayout.SetLoadFields(Name);
if TenantReportLayout.FindSet() thenKnowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4 |
Co-authored-by: MSlenejennum <276599928+MSlenejennum@users.noreply.github.com>
github-actions
Bot
added
needs-approval
…eport-Themes' of https://github.com/microsoft/BCApps into MSlenejennum/647452/New-Header-and-Footer-Layouts-and-Report-Themes
Three defects behind the two failing composite layout tests. ClearAssignments built its Report Layout List record and set Application ID to the app the part was seeded under. An assignment references the Application ID that Report Layout List reports, which is the empty GUID for a part stored in Tenant Report Layout, so the key never matched and nothing was cleared. It now looks the part up instead. SeedPart relied on TryGetPartLayout to catch a missing resource, but NavApp.GetResource raises an error a try function cannot catch, so ResourceNotReadableErr was never reached. NavApp.ListResources decides whether the resource is there before it is read. The error stays ErrorType::Internal, so the message goes to telemetry rather than to the user. Message is composed with StrSubstNo, which telemetry replaces with a placeholder, so the part name and the resource are carried in CustomDimensions where they survive. SeedPartRaisesWhenTheResourceIsMissing no longer asserts on the message. An Internal error shows a generic text and keeps the message out of GetLastErrorText, so no assertion can reach it. That it raises, and that no half-seeded row is left, is what stays observable.
1435351 to
cef1145
Compare
The error builder always appended GetLastErrorText, which reads as an empty platform error on the path where the resource is simply not in the app - there is no platform failure to report there. The builder now takes the detail as a parameter, so a missing resource says so and an unreadable one still carries the platform error. Also collapses a double blank line left between two test methods.
Good Sense ReviewRecommendation: Accept No actionable suggestions. I did not cast a formal review vote.
|
| var | ||
| UpgradeCompositeReportParts: Codeunit "Upgrade Composite Report Parts"; | ||
| begin | ||
| UpgradeCompositeReportParts.SeedShippedParts(); |
There was a problem hiding this comment.
there seems to be two paths seeding the new report parts:
- upgrade
- oninstall of the app (baseapp)
We usually add also OnCompanyOpen, or something that populates on-demand. The reason is that new tenants get provisioned with an image of the database that has BaseApp installed and didn't go necessarily through the oninstall trigger.
Add such third path please.
There was a problem hiding this comment.
btw, please be aware, if you add this other path, take care of the permissions involved, given that in that context there's no elevated permissions, so before doing that, check the needed permissions (we don't want something like this block OnCompanyOpen)
Predrag Maricic (PredragMaricic)
left a comment
Predrag Maricic (PredragMaricic)
left a comment
There was a problem hiding this comment.
The shipped Word packages still contain machine-local external template relationships that need to be removed before these resources are distributed with BaseApp.
Eight packages contain an attachedTemplate relationship in word/_rels/settings.xml.rels targeting unavailable local files such as file:///C:\Users\jacobwj\Desktop\Simplified Sales Document.dotx, file:///C:\Users\SarahBachBendsen\Downloads\..., or file:///C:\depot\NAV\...\Calm.dotx:
Default.dotxExternalDefault.docxExternalDefaultDetailed.docxExternalMinimalistic.docxExternalMinimalisticDetailed.docxExternalModern.docxInternalDefault.docxInternalMinimalisticCentered.docx
This leaks contributor/local build paths into shipped content and can make Word attempt to resolve templates that do not exist on customer machines. Please remove these relationships and smoke-test the cleaned packages through the composition/opening flow.
Also, please optimize or justify Playful.dotx: it is 5,459,764 bytes versus about 56 KB for each sibling theme, with one 2043x2749 PNG accounting for 5,411,919 bytes. The resource is carried in the app and imported into Tenant Report Layout per database, so avoiding this roughly 97x size difference would reduce package and database bloat.
The AL seeding, ownership, pruning, upgrade-tag gating, localization propagation, and regression coverage otherwise look coherent, and the current checks are green.
| /// the single entry point that does both; RunUpgrade wraps it in the tag guard so an upgrade runs it once, while | ||
| /// install calls it unguarded. Add a new dated tag whenever the shipped layout files change. | ||
| /// </summary> | ||
| codeunit 104064 "Upgrade Composite Report Parts" |
There was a problem hiding this comment.
we have a place where we usually put BaseApp upgrades, please follow that
Joshua (mynjj)
left a comment
Joshua (mynjj)
left a comment
There was a problem hiding this comment.
"not gated by its upgrade tag — runs on every upgrade and overwrites tenant edits."
doesn't seem like what the code says, I don't have very clear what's the story for updating these layouts if changes are needed after seeded.
"logged to telemetry and skipped, not raised."
Seems like Error is used
What is the actual intended behavior?
Please also consider telemetry to the management operations.
|
AB#647452
Seed shipped header/footer and report theme parts on install and upgrade
Adds the out-of-box Composite Layout parts (11 header/footer designs, 3 themes) and the
install/upgrade pass that writes them into the shared pool under Tenant Report Defaults. The page
and lookup helper are already in
main; this supplies their content.New:
codeunit 9667 "Composite Report Parts Mgt.",codeunit 104064 "Upgrade Composite Report Parts", 14 resources under.resources/ReportParts/.Changed:
BaseAppInstallseeds on install;UpgradeTagDefinitions(W1/BE/IT/RU) registers theper-database tag;
CompositeLayoutTestsnow 42 tests.Behaviour
NavApp.GetCurrentModuleInfo. That IDis part of the
Tenant Report Layoutkey and of the<guid>::<name>reference every assignmentstores.
content be modified.
to the shipped parts. Deliberate: these are app content, not tenant data. Copy a part to make a
variant.
SeedPartcall actually takes it out of the pool.
error here would abort install or upgrade.